Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Make event types control the lifecycle of capture and playback #36

Closed
wants to merge 13 commits into from

Conversation

snendev
Copy link
Collaborator

@snendev snendev commented Mar 1, 2024

Issue

#34 - Refactor to allow users to trigger capture/playback as-needed

I've made this a draft PR for now since it needs tests for the new features. I thought it would be better to write those after submitting a working version so that the details can be ironed out first.

Changes

The strategy was to define new Event types that would be used to control the lifecycle of capture/playback operations. These are effectively [Begin|End]Input[Capture|Playback]. The Begin types each require the data to build its own corresponding Resources; the End types carry no data.

Plugins no longer attach TimestampedInputs or other resources related to capture/playback. Instead, these resources and attached and removed as capture and playback actions are detected.

This also adds a mechanism for overwriting the window entity that inputs target, in case a specific window should be targeted on playback. This will be useful in editor or multi-window contexts and is generally nice to have, so it fits well in this set of changes.

I also deleted the random file I added in the last PR. My bad!

Questions

  • Many of the systems have run conditions attached, but it is somewhat arbitrary from a development perspective which Resource "should" be included in run conditions. Do you have strong opinions here? Does the system ordering seem correct?
  • Since resource existence now triggers a lot of the behavior, it's not clear whether PlaybackFilePath still needs to accept an Option.
  • What should happen if users want to keep TimestampedInputs in the world after capture? A new-type Resource for TimestampedInputs could be appropriate.

TODOs

  • Need to write tests for the new lifecycles and various Resources.
  • Need to read deserialization errors and forward them to clients (probably another Event).

@JeanMertz
Copy link

@alice-i-cecile any chance of giving this a review and possible merge in the near future?

I'm interested in using this crate (and pushing a 0.14 upgrade), but seeing this PR linger for 5 months has me worried that (understandably, given your new role in Bevy) this is on the back burner, and I'm better off either forking or helping out with bevy-autoplay (which is less expansive as this crate currently, so my preference would be for this crate to get some TLC).

@snendev
Copy link
Collaborator Author

snendev commented Jul 16, 2024

@JeanMertz @alice-i-cecile I'll try to push this further along ASAP as well and add the tests that are needed for it to be merge-ready. happy to handle review comments as I do so if you get a chance Alice. it might take me a couple days since I have a couple other things going on.

I do also have some work done on the 0.14 upgrade as well ATM, just need to figure out the correct approach for the events-based tests now that the story around EventUpdateSignal resource in bevy works differently. (I had to fix some problems there when upgrading to 0.12.) I'll get back to that ASAP so it can be ready soon.

@snendev
Copy link
Collaborator Author

snendev commented Aug 22, 2024

Just added some updates to merge main and update some tests. Not quite done here yet -- still need to add the new tests. But at least current tests pass. Will do my best to complete this soon!

@JeanMertz
Copy link

@snendev I might have a use-case for this in the coming weeks. I could help push this over the finish line, unless you have any uncommitted work you're actively working on, or want to wrap this up soon?

With your blessing, please let me know what's still pending for this work to be good to go, other than adding new tests.

@snendev
Copy link
Collaborator Author

snendev commented Nov 14, 2024

My apologies for the delay here! Thanks @JeanMertz for the ping.

If I remember right, I realized at some point that Observers were probably more suited for this than Events anyway, and then I ended up down some deep rabbit hole instead of testing the new behavior. Then I went on a vacation and completely forgot about this. Really sorry about that!

I'll try to put up the commits I have cooking tonight -- tbh it kinda implies another big refactor though. I will probably put those changes up on a separate branch for independent review that way we can compare the solutions more easily, unless I really think one is much stronger than the other. Once it's clear what direction is preferred I would definitely appreciate help adding any tests we think are useful.

Sorry again for the delay! I'll put up the code ASAP.

@JeanMertz
Copy link

No worries @snendev , and it does appear that observers would be the way to go on first sight.

@snendev snendev marked this pull request as ready for review November 17, 2024 03:04
@snendev
Copy link
Collaborator Author

snendev commented Nov 17, 2024

@JeanMertz please compare with #39. Sorry for the delay, I got caught up in the ShouldEventsUpdate stuff messing up tests (which has happened to me in this crate before) -- haven't recently investigated why it works in this PR without that code, but either way, it's appropriate for the test suite.

@snendev
Copy link
Collaborator Author

snendev commented Dec 2, 2024

--> #39!

@snendev snendev closed this Dec 2, 2024
@snendev snendev deleted the controlled-execution branch December 2, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants